-
Notifications
You must be signed in to change notification settings - Fork 622
✨ Migrate Node packages to AWS SDK v2 #5584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Migrate Node packages to AWS SDK v2 #5584
Conversation
Welcome @LiangquanLi930! |
Hi @LiangquanLi930. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
@damdo Hi, I have a question I'd like to ask — regarding cluster-api-provider-aws/pkg/cloud/interfaces.go Lines 33 to 38 in 701052c
as part of this issue we’re trying to address? |
Hey @LiangquanLi930 I don't think so, but better to ask @punkwalker |
I saw #5521 — maybe we should wait for this PR to merge before proceeding with ours. |
Yeah, that's probably better, let's pause this until we get the other in, thanks @LiangquanLi930 |
@LiangquanLi930 @damdo The session() can only be removed if all the dependent clients are migrated to SDK V2. Let’s wait for #5521 to merge and then cleanup every reference to SDK v1 sessions. |
462b877
to
4c85641
Compare
@LiangquanLi930 looks like there is a compilation error |
4c85641
to
1e22f5e
Compare
Still present it looks like @LiangquanLi930 |
I am taking it now |
cd2370f
to
191a56f
Compare
/test pull-cluster-api-provider-aws-test |
1 similar comment
/test pull-cluster-api-provider-aws-test |
89043d1
to
9c4c8f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @LiangquanLi930
@punkwalker would you be able to take a look and confirm this looks in line with what you expect to see? Thanks!
/assign @nrb for review |
/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e |
/label tide/merge-method-squash I'll set this to squash automatically, but in the future, try to squash changes into a single commit, especially when removing debugging commits. |
@@ -158,6 +158,10 @@ func Node1BeforeSuite(e2eCtx *E2EContext) []byte { | |||
e2eCtx.Environment.BootstrapAccessKey = newUserAccessKey(context.TODO(), e2eCtx.AWSSessionV2, bootstrapTemplate.Spec.BootstrapUser.UserName) | |||
e2eCtx.BootstrapUserAWSSession = NewAWSSessionWithKey(e2eCtx.Environment.BootstrapAccessKey) | |||
e2eCtx.BootstrapUserAWSSessionV2 = NewAWSSessionWithKeyV2(e2eCtx.Environment.BootstrapAccessKey) | |||
|
|||
By("Waiting for access key to propagate...") | |||
time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a condition that we can use an Eventually
on, versus a static 10s wait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, In this case, there isn’t an easily queryable condition to verify key propagation. The static wait ensures we avoid flakiness from IAM eventual consistency, so replacing it with Eventually wouldn’t provide a clear benefit.
And other parts of the code handle it the same way as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cluster-api-provider-aws/test/e2e/suites/unmanaged/unmanaged_functional_test.go
Lines 502 to 504 in c04842a
ginkgo.By("Deleting a worker node machine") | |
deleteMachine(ns1, md1[0]) | |
time.Sleep(10 * time.Second) |
pkg/cloud/scope/cluster.go
Outdated
@@ -77,7 +77,7 @@ func NewClusterScope(params ClusterScopeParams) (*ClusterScope, error) { | |||
maxWaitActiveUpdateDelete: params.MaxWaitActiveUpdateDelete, | |||
} | |||
|
|||
session, serviceLimiters, err := sessionForClusterWithRegion(params.Client, clusterScope, params.AWSCluster.Spec.Region, params.Endpoints, params.Logger) | |||
serviceLimiters, err := sessionForClusterWithRegion(params.Client, clusterScope, params.AWSCluster.Spec.Region, params.Endpoints, params.Logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiangquanLi930 I see you have removed this v1 code in other scopes like ManagedNdoeGroup. Can we do the same here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9c4c8f4
to
bf14108
Compare
/test pull-cluster-api-provider-aws-e2e-eks pull-cluster-api-provider-aws-e2e |
/retest |
/approve |
LGTM label has been added. Git tree hash: b7834309a3998dc6656f03bb7c0a144100470d55
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@LiangquanLi930: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What type of PR is this?
Add one of the following kinds:
/kind feature
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #5402
Special notes for your reviewer:
Checklist:
Release note: